Skip to content

fixed #13982 - cppcheck.cpp: do not crash if mExecuteCommand is empty #7647

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

firewave
Copy link
Collaborator

@firewave firewave commented Jul 6, 2025

No description provided.

@firewave firewave changed the title refs #13982 - cppcheck.cpp: do not crash if mExecuteCommand is empty fixed #13982 - cppcheck.cpp: do not crash if mExecuteCommand is empty Jul 6, 2025
@firewave
Copy link
Collaborator Author

firewave commented Jul 6, 2025

This only fixes the crash when running an scratchpad analysis with a loaded projects including addons. It will now show an error instead.

The execute function is not available in that context and it requires some refactoring. But I think that is something to look into in the context of https://trac.cppcheck.net/ticket/13976.

@firewave

This comment was marked as resolved.

@firewave firewave marked this pull request as draft July 6, 2025 22:42
@firewave firewave marked this pull request as ready for review July 6, 2025 23:18
@firewave firewave marked this pull request as draft July 7, 2025 09:35
@firewave firewave force-pushed the scratch-addon branch 4 times, most recently from 988764d to e6a6e24 Compare July 7, 2025 11:00
@firewave
Copy link
Collaborator Author

firewave commented Jul 7, 2025

I moved the checks from the executor tests as the code being tested was not specific to those classes and actually lives in CppCheck.

@firewave firewave marked this pull request as ready for review July 7, 2025 11:50
@@ -686,6 +689,11 @@ unsigned int CppCheck::checkClang(const FileWithDetails &file, int fileIndex)
mErrorLogger.reportOut(exe + " " + args2, Color::Reset);
}

if (!mExecuteCommand) {
std::cerr << "Failed to execute '" << exe << " " << args2 << " " << redirect2 << "' - (no command callback provided)" << std::endl;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why isn't this reported to the errorlogger. GUI does not capture stderr. And plugins will probably not write this. So the user will have no clue why it does not work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because that was never properly implemented for this. It's the same in another cases and there's TODOs in the code. Will check if we have tickets about this.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what I am missing. What is stopping you from calling mErrorLogger.reportError here instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what I am missing. What is stopping you from calling mErrorLogger.reportError here instead?

It just was never implemented that way back when this functionality was introduced. That is outside of the scope of this PR.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
34.0% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@firewave
Copy link
Collaborator Author

[![Quality Gate Failed]

Great - this is back. Now any build will show up as failed after it has been merged...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants